refactor(chartverifier): simplify GetPackageDigest and document helpers#608
Conversation
|
Thanks for your pull request! A maintainer will review this pull request and trigger functional testing by adding the ok-to-test label. This comment was auto-generated by GitHub Actions. |
| // openChartPackageReader opens the chart package byte stream at u for digesting. | ||
| // On success, it returns an [io.ReadCloser]: for http or https, the response body from [http.Get]; | ||
| // for file or an empty scheme, the file from [os.Open] when the URL path ends with ".tgz". | ||
| // It returns (nil, nil) for file or an empty scheme when the path does not end with ".tgz". | ||
| // It returns (nil, err) if the scheme is unsupported, or if [http.Get] or [os.Open] fails. |
There was a problem hiding this comment.
This feels redundant, seeing it both in the exported Function as well as here, even though they have subtle differences. Can we figure out a way to make sure the comments don't overlap as heavily?
There was a problem hiding this comment.
Also, feels a little too specific by providing exact error case tuples as references. Better to capture the behavior at a higher level, and leave the return cases as something that can be observed by reading the logic itself, given it isn't very long.
There was a problem hiding this comment.
I've shortened/condensed the godoc in both functions.
komish
left a comment
komish
left a comment
There was a problem hiding this comment.
Few comments. Overall, I'm not sure if we're getting too much here given we don't really enrich or use any of the errors that would be returned by the new function, but if you believe it's more readable, we can merge this after the requested changes.
caxu-rh
force-pushed
the
fix-reportbuilder-close
branch
from
960b797 to
417e5b8
Compare
github-actions
Bot
removed
the
ok-to-test
|
Thanks for the feedback. The goal was originally to ensure that reader (file or response body) gets closed, but due to the early-return style it would've required multiple |
Extract openChartPackageReader for clearer scheme handling and a single defer rc.Close() on success. Propagate os.Open errors instead of ignoring them. Add godoc for GetPackageDigest and openChartPackageReader. Co-authored-by: Cursor <cursoragent@cursor.com>
caxu-rh
force-pushed
the
fix-reportbuilder-close
branch
from
417e5b8 to
f78fa15
Compare
github-actions
Bot
removed
the
ok-to-test
2 participants
Extract openChartPackageReader for clearer scheme handling and a single defer rc.Close() on success. Propagate os.Open errors instead of ignoring them. Add godoc for GetPackageDigest and openChartPackageReader.